Move navigation and selection logic to WritingFlow #19397

ellatrix · 2020-01-03T11:10:03Z

Description

This PR moves selection and navigation mode logic (event listeners) to WritingFlow.

This removes a lot of event listeners that are currently added to each block.
The functions are recreated less often since WritingFlow is rerendered less often. These functions don't depend on any block data, only on selection state.
This removes the need for the IgnoreNestedEvents component.
Creates a RootContainer component where I moved some common block logic and useMultiSelection (which is now only rendering once).
It simplifies the BlockListBlock component. The absence of the event handlers will also make it easier later on to remove the div wrappers.
We may want to refactor WritingFlow a little to separate navigation and selection logic.

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

ellatrix · 2020-01-03T12:10:33Z

packages/block-editor/src/components/block-list/block.js

@@ -469,18 +401,15 @@ function BlockListBlock( {
 	);

 	return (
-		<IgnoreNestedEvents


@aduth Might be of interest to you as you originally created this component.

youknowriad · 2020-01-03T14:09:11Z

Did you test the performance impact of this PR. It seems like it has potential for improving the numbers.

ellatrix · 2020-01-03T14:12:02Z

@youknowriad I did a quick check and I think it does improve slightly, but I have to say that, on my computer, the number vary even when I test the master branch a few times in a row. 🤷‍♀️

aduth · 2020-01-03T14:34:05Z

Related (would make redundant): #14707

aduth · 2020-01-03T15:01:37Z

One thing this has me wondering is whether we need to start considering to bake in WritingFlow as part of the default rendering of a BlockEditor. Right now, it's a separate, optional component. But for a naive rendering of a block editor, the changes here would have the effect that a lot of the interactivity is lost.

I guess we document this as more-or-less assumed to be part of a standard rendering hierarchy, so the expectation is already there. Furthermore, we rely on this in the top-level post editor to be able to include the post title as part of the consideration of the writing flow.

aduth · 2020-01-03T15:21:40Z

One thing this appears to regress on is that clicking the button block appender in a column block will first select that block before allowing the inserter to be shown. This has been a source of some trouble in the past (e.g. #18329), where the intended behavior is that clicking the appender button should immediately open the inserter.

Steps to reproduce:

Navigate to Posts > Add New
Insert a Columns block
Select any columns arrangement
Unselect the Columns block
Click one of the appender buttons in any of the columns

Expected: The inserter is opened.
Actual: The block is selected. A second click is required to open the inserter.

Related: #19135, #18379

There are end-to-end in both of those pull requests. I think it might be that the navigation block works correctly (does appear to in my testing, not sure why it's any different). The tests modified in #19135 are primarily to test against regressions to the expected behavior described in #18928. It might be that we lack a test case specifically verifying this columns (button) appender behavior.

aduth · 2020-01-03T15:30:29Z

I generally like the direction this is taking, and am especially fond of the idea of removing the complexities surrounding IgnoreNestedEvents in favor of handling block selection events further up the virtual tree. One worry I might have is whether WritingFlow is starting to do too much, at least in being able to succinctly define its purpose. It's conflicting to me because I do think some of the refactoring could make sense in WritingFlow (e.g. multi-selection, navigation mode toggling). I'm not sure of it all, though, such as the focus handling, at least if we consider WritingFlow as having primarily served the purpose of handling keyboard interactions with respect to their implications on block selection.

Maybe this is a matter of my own unclear expectations around what this component is meant to be responsible for handling. I think it's worth clarifying in any case (ideally, though not necessarily as part of this pull request, as documentation for the component).

Alternatively, it's something where we could consider splitting off some of the specific functionality into separate components. This could be relevant to my previous point about "default" behaviors, where if we don't want to bake-in all of WritingFlow, we could still split out some of the functionality to become default. In my head, I'm thinking of components like MultiSelectScrollIntoView, which itself is somewhat related to the functionality of WritingFlow, but isolated for the purpose of improving clarity around what it's expected to be handling.

aduth · 2020-01-03T15:32:47Z

On the topic of #19397 (comment) and default behaviors, I wonder if this might have any impact on whether we will be able to implement nested block editors (e.g. #14715, I think also Widgets screen might have some nested editors?). My worry is: How does selection work for those inner editors, if it's meant to be handled by WritingFlow? Does it mean there would need to be multiple WritingFlow, one for each of the BlockEditorProviders?

ellatrix · 2020-01-03T16:10:23Z

I think it might be good to split WritingFlow in multiple components, e.g. one for multi-selection, one for navigation, and one for focus handling. Currently WritingFlow already does some focus handling for moving it in and out the block.

I also like baking WritingFlow, or some parts of it, in the block editor.

I'm sot sure how nested editors would work, and I'm sure we'd have to revise the currently selection logic and WritingFlow as well. Should you be able to select and navigate across editors? If yes, then there should probably be one WritingFlow component wrapping everything.

ellatrix · 2020-01-03T16:11:23Z

I'll fix the remaining issue with the column inserter and attempt to split WritingFlow in parts.

aduth · 2020-01-03T16:49:40Z

Should you be able to select and navigate across editors? If yes, then there should probably be one WritingFlow component wrapping everything.

I don't think it's necessarily a requirement, especially considering that if each editor is responsible for maintaining its own blocks state, I don't know how a top-level WritingFlow could possibly track the selection for those blocks (since, at that level, the blocks state is not known).

ellatrix · 2020-01-04T13:50:03Z

I've now moved the focus handling to the root BlockList so it is backed in. The other stuff like navigation mode and multi selection logic do make sense in WritingFlow so I left it there.

ellatrix · 2020-01-04T14:02:55Z

packages/block-editor/src/components/block-list/index.js

@@ -72,18 +71,17 @@ function BlockList( {
 		hasMultiSelection,
 		enableAnimation,
 	} = useSelect( selector, [ rootClientId ] );
-	const ref = useRef();
-	const onSelectionStart = useMultiSelection( { ref, rootClientId } );


Notice that this is no longer rendered for every block list, but rather only for the root block list.

ellatrix · 2020-01-04T14:05:29Z

packages/block-editor/src/components/writing-flow/index.js

 	} = useDispatch( 'core/block-editor' );

+	function onMouseDown( event ) {


The logic below belongs here in my opinion, as this component handles multi selection as well as navigation mode already. Perhaps we can split this is the future, but a lot of it seems tied together.

ellatrix · 2020-01-06T11:02:28Z

packages/block-editor/src/components/block-list-appender/index.js

+			tabIndex={ -1 }
+			// Prevent the block from being selected when the appender is
+			// clicked.
+			onFocus={ stopPropagation }


@aduth This fixed the issue you found (#19397 (comment)). Does everything now work correctly?

Initial impression: I imagine this could work, yes, considering that IgnoreNestedEvents was in many ways a glorified stopPropagation anyways. Will test as part of a broader re-review.

Thanks! It would be nice the eventually get rid of this as well. I'll look into it sometime separately.

Thanks! It would be nice the eventually get rid of this as well. I'll look into it sometime separately.

Yeah, that's fair. I'm very reluctant about any stopPropagation, but I think it's fair to say it's not really "introduced" here, since it was more-or-less the same effect previously.

Right, same here. At least it's now explicit that propagation is stopped here. IgnoreNestedEvents kind of made it look like it's more ok.

Right, same here. At least it's now explicit that propagation is stopped here. IgnoreNestedEvents kind of made it look like it's more ok.

It's a give and take, I think. One nice thing about IgnoreNestedEvents is that it wouldn't stop all propagation, only that which it expressly cares about, thus potentially avoiding issues with other (intended) event handling further up the element hierarchy.

aduth

I ran through a number of scenarios relating to the affected code. There's a lot of potential impact here, but as best I can tell, it's all in good shape.

While I'm approving, I do think at the very least we should resolve the issue with Node.ELEMENT_NODE vs. node.ELEMENT_NODE.

It's a little concerning to me how much we rely on the DOM as the source of truth here with the introduction and reliance on getBlockClientId, but I also don't think it contradicts how we've approached WritingFlow to date, and it might be worth the compromise for all the benefits/simplification it brings.

aduth · 2020-01-07T15:57:08Z

packages/block-editor/src/components/block-list/root-container.js

+		hasMultiSelection,
+	} = useSelect( selector, [] );
+	const { selectBlock } = useDispatch( 'core/block-editor' );
+	const onSelectionStart = useMultiSelection( ref );


(Thinking out loud, since it seems fine, but worth confirming) I suppose that because useMultiSelection returns a value via useCallback (with memoization) that this shouldn't be a performance concern in changing values in the provided value below (reference)?

Right, it's memoized with useCallback, so it should be fine.

aduth · 2020-01-07T16:00:32Z

packages/block-editor/src/components/block-list/use-multi-selection.js

-			getBlockParents,
-		};
-	}
+function selector( select ) {


Aside: While the separate function declaration still catches me off guard, one thing which is nice here is demonstrating how, if you are able to move it outside the component declaration altogether, there's a safer guarantee that you don't need to provide any dependencies to the useSelect call (since there's no longer the component scope from which to inherit dependencies).

I agree. It's maybe a bit strange to put it outside the component, but why not if you can prevent creating an additional function on every render.

aduth · 2020-01-07T16:07:55Z

packages/block-editor/src/components/writing-flow/index.js

+					}
+				// Allow user to escape out of a multi-selection to a singular
+				// selection of a block via click. This is handled here since
+				// onFocus excludes blocks involved in a multiselection, as


Nit: This was copied from BlockListBlock, but since "onFocus" doesn't exist in this file, it's a bit confusing to read now.

packages/block-editor/src/utils/dom.js

aduth · 2020-01-07T16:12:44Z

packages/block-editor/src/utils/dom.js

+ * @return {?string} Client ID or undefined if the node is not part of a block.
+ */
+export function getBlockClientId( node ) {
+	if ( node.nodeType !== node.ELEMENT_NODE ) {


Unless I'm mistaken, this should be:

Suggested change

if ( node.nodeType !== node.ELEMENT_NODE ) {

if ( node.nodeType !== Node.ELEMENT_NODE ) {

https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType#Constants

Currently, I believe it will always be triggering the condition, since node.ELEMENT_NODE would evaluate to undefined.

I think this is correct. Every node instance has these properties.

I think this is correct. Every node instance has these properties.

Ah, interesting. I did not know this.

(For posterity's sake: https://dom.spec.whatwg.org/#interface-node)

aduth · 2020-01-07T16:15:17Z

packages/block-editor/src/components/block-list/block.js

+				// Only allow selection to be started from a selected block.
+				onMouseLeave={ isSelected ? onMouseLeave : undefined }


Is this related to multi-selection? And if it is, would any of it be expected to be handled in WritingFlow instead? Considering you mention it at https://github.com/WordPress/gutenberg/pull/19397/files#r363036970 as being partly responsible for handling multi-selection.

That's right, this is for multi selection. It would be nice to move this to WritingFlow as well. I didn't do it here to avoid a huge PR, as it seems a little more tricky to solve.

aduth · 2020-01-07T16:36:08Z

packages/block-editor/src/components/block-list-appender/index.js

+			// Prevent the block from being selected when the appender is
+			// clicked.


I was actually surprised to find that in the latest version of the plugin / core version, the behavior of clicking on the inserter in the column block is such that it requires a second click to open. I don't think it's always been like this, since I distinctly remember trying to require only a single click. One potential difference now is that, while it does only require a single click, it doesn't actually select the block. I don't particularly see this as being problematic, and in some ways it's actually an ideal behavior (since the user's intent is to insert, not necessarily to select the block).

Co-Authored-By: Andrew Duthie <[email protected]>

ellatrix · 2020-01-07T16:48:11Z

It's a little concerning to me how much we rely on the DOM as the source of truth here with the introduction and reliance on getBlockClientId, but I also don't think it contradicts how we've approached WritingFlow to date, and it might be worth the compromise for all the benefits/simplification it brings.

I agree. I have some future improvements in mind around keeping the block references higher up the tree that could eliminate having to query the DOM. Not sure if and how it will work, but I generally agree that it would be nice to avoid any queries.

aduth · 2020-01-08T19:49:01Z

I was actually surprised to find that in the latest version of the plugin / core version, the behavior of clicking on the inserter in the column block is such that it requires a second click to open. I don't think it's always been like this, since I distinctly remember trying to require only a single click. One potential difference now is that, while it does only require a single click, it doesn't actually select the block. I don't particularly see this as being problematic, and in some ways it's actually an ideal behavior (since the user's intent is to insert, not necessarily to select the block).

One thing I've noticed about the new behavior: When inserting a Columns block and immediately proceeding to try to adjust the widths of individual columns, it can now be more difficult to select the columns, where previously it would be enough to click the button inserter, even if it wasn't the intention to actually insert a block. Granted, "Clicking the inserter" for the purpose of selecting a block is itself a bit of an awkward expectation and probably not the desired behavior, but for lack of another option, it was at least more consistent than it is currently.

ellatrix added the [Status] In Progress Tracking issues with work in progress label Jan 3, 2020

ellatrix requested review from talldan and youknowriad as code owners January 3, 2020 11:10

ellatrix added the [Type] Performance Related to performance efforts label Jan 3, 2020

ellatrix commented Jan 3, 2020

View reviewed changes

ellatrix requested a review from aduth January 3, 2020 13:04

ellatrix added [Type] Code Quality Issues or PRs that relate to code quality and removed [Status] In Progress Tracking issues with work in progress labels Jan 3, 2020

ellatrix force-pushed the try/move-nav-logic branch from 7e7faf5 to 96fd5fb Compare January 3, 2020 13:34

ellatrix changed the title ~~[WIP] Move navigation and selection logic to WritingFlow~~ Move navigation and selection logic to WritingFlow Jan 3, 2020

ellatrix commented Jan 4, 2020

View reviewed changes

ellatrix commented Jan 6, 2020

View reviewed changes

ellatrix added 6 commits January 6, 2020 15:53

WritingFlow: absorb navigation logic from block

f4727bc

Move selection and navigation logic to WritingFlow

5c1bdba

Use getBlockClientId in useMultiSelection

5fb2ddf

Fix block dragging

dadd96d

Fix focus after escape to nav mode

815e7ca

Remove IgnoreNestedEvents

8178ca9

ellatrix added 7 commits January 6, 2020 15:56

Add getBlockClientId docs

bdd923f

Restore tabindex comment

43ca6ea

Fix nested block appender click

9ad9c44

Move focus and drag handling

90d30ca

Create root container for block list

c1239e3

Polish

dbfb783

Conditionally bind block event handlers

6c6acc0

ellatrix force-pushed the try/move-nav-logic branch from 47a414f to 6c6acc0 Compare January 6, 2020 14:58

Ensure multiselection has 2 items before selecting

82be361

aduth approved these changes Jan 7, 2020

View reviewed changes

Update packages/block-editor/src/utils/dom.js

3ddc1d5

Co-Authored-By: Andrew Duthie <[email protected]>

Adjust onFocus comment

0491a00

ellatrix merged commit 4ba6ca0 into master Jan 8, 2020

ellatrix deleted the try/move-nav-logic branch January 8, 2020 08:55

ellatrix mentioned this pull request Jan 8, 2020

Block list appender: use prevent default to prevent block selection #19503

Closed

6 tasks

aduth mentioned this pull request Jan 9, 2020

Block Library: Avoid column width auto-adjustment when sibling width changes #19515

Merged

ellatrix mentioned this pull request Jan 10, 2020

Block: move keydown event to rootContainer #19575

Closed

6 tasks

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

aduth mentioned this pull request Feb 25, 2020

Export IgnoreNestedEvents component #14707

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move navigation and selection logic to WritingFlow #19397

Move navigation and selection logic to WritingFlow #19397

ellatrix commented Jan 3, 2020 •

edited

Loading

ellatrix Jan 3, 2020

youknowriad commented Jan 3, 2020

ellatrix commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

ellatrix commented Jan 3, 2020

ellatrix commented Jan 3, 2020

aduth commented Jan 3, 2020

ellatrix commented Jan 4, 2020

ellatrix Jan 4, 2020

ellatrix Jan 4, 2020

ellatrix Jan 6, 2020

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020

aduth left a comment

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020 •

edited

Loading

aduth Jan 7, 2020

ellatrix Jan 7, 2020

aduth Jan 7, 2020

ellatrix commented Jan 7, 2020

aduth commented Jan 8, 2020

		} = useDispatch( 'core/block-editor' );

		function onMouseDown( event ) {

	if ( node.nodeType !== node.ELEMENT_NODE ) {
	if ( node.nodeType !== Node.ELEMENT_NODE ) {

		// Only allow selection to be started from a selected block.
		onMouseLeave={ isSelected ? onMouseLeave : undefined }

		// Prevent the block from being selected when the appender is
		// clicked.

Move navigation and selection logic to WritingFlow #19397

Move navigation and selection logic to WritingFlow #19397

Conversation

ellatrix commented Jan 3, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

youknowriad commented Jan 3, 2020

ellatrix commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

aduth commented Jan 3, 2020

ellatrix commented Jan 3, 2020

ellatrix commented Jan 3, 2020

aduth commented Jan 3, 2020

ellatrix commented Jan 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jan 7, 2020

aduth commented Jan 8, 2020

ellatrix commented Jan 3, 2020 •

edited

Loading

aduth Jan 7, 2020 •

edited

Loading